syscontainers: set SELinux label for files copied to the host #1143

giuseppe · 2017-12-06T18:09:43Z

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

giuseppe · 2017-12-06T20:32:11Z

@jeremyeder this should also fix the issue we were discussing last week with files copied to the host not getting the right label

cgwalters · 2017-12-06T22:26:25Z

Atomic/rpm_host_install.py

@@ -25,6 +26,10 @@ def copyfile(src, dest):
            return True
        else:
            shutil.copy2(src, dest)


It's more robust to use setfscreatecon() to atomically create the file with the right context; see what ostree-sepolicy.c does.

thanks for the hint, I'll give it a try and update the patch. I'll need to check the "copy2" doesn't override it.

peterbaouoft · 2017-12-07T15:35:59Z

Atomic/rpm_host_install.py

@@ -52,7 +57,7 @@ def file_checksum(path, blocksize=(1<<20)):
        return h.hexdigest()

    @staticmethod
-    def rm_add_files_to_host(old_installed_files_checksum, exports, prefix="/", files_template=None, values=None, rename_files=None):
+    def rm_add_files_to_host(old_installed_files_checksum, exports, prefix="/", files_template=None, values=None, rename_files=None, files_content=None):


hi, quick question, is files_content used? it seems to fail the pylint check when running normal make for upstream repo(forgot to mention it yesterday)

[root@localhost atomic]# make /usr/bin/python setup.py build running build running build_py copying Atomic/rpm_host_install.py -> build/lib/Atomic running build_scripts /usr/bin/python -m pylint --disable=all --enable=E --enable=W --additional-builtins=_ *.py atomic Atomic tests/unit/*.py -d=no-absolute-import,print-statement,no-absolute-import,bad-builtin,catching-non-exception,raising-non-exception No config file found, using default configuration ************* Module Atomic.rpm_host_install W: 60,133: Unused argument 'files_content' (unused-argument) -------------------------------------------------------------------- Your code has been rated at 10.00/10 (previous run: 10.00/10, -0.00) make: *** [Makefile:41: pylint-check] Error 4

thanks to have noticed it. It is a piece from another patch I was working on, I dropped it in a fixup patch

giuseppe · 2017-12-08T12:00:38Z

I've changed the implementation to use setfscreatecon_raw. I've marked the PR as WIP as I have not added a new test yet.

@peterbaouoft could you please verify if the firewalld system container still work with the new implementation and that the new files get a correct label?

peterbaouoft · 2017-12-08T14:36:03Z

Atomic/rpm_host_install.py

@@ -74,7 +79,14 @@ def rm_add_files_to_host(old_installed_files_checksum, exports, prefix="/", file
        # if there is a directory hostfs/ under exports, copy these files to the host file system.
        hostfs = os.path.join(exports, "hostfs")
        new_installed_files_checksum = {}
-        if os.path.exists(hostfs):
+        if not os.path.exists(hostfs):
+            return new_isntalled_files_checksum


could you please verify if the firewalld system container still work with the new implementation and that the new files get a correct label?

sure thing =)

Tho, seems like a typo here, shouldn't it be new_installed_files_checksum?

+ make pylint-check python3 -m pylint --disable=all --enable=E --enable=W --additional-builtins=_ *.py atomic Atomic tests/unit/*.py -d=no-absolute-import,print-statement,no-absolute-import,bad-builtin,catching-non-exception,raising-non-exception No config file found, using default configuration ************* Module Atomic.rpm_host_install E: 83,19: Undefined variable 'new_isntalled_files_checksum' (undefined-variable)

:/ yes thanks. Fixed in a newer version

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2017-12-15T13:45:18Z

@peterbaouoft from my tests here, it solves the issue. Have you had a chance to verify this on your end?

peterbaouoft · 2017-12-15T14:56:23Z

@giuseppe , currently checking. The last time steve and I verified the first stage systemctl start firewalld will be able to start without the selinux problem after applying the patch fix.

However, recently I checked on the host I had, I am unable to run atomic run firewalld firewall-cmd --state saying that I get org.fedoraproject.FirewallD1.info is not registered. (even if systemctl start firewalld succeeds )

EDIT: Thanks for the fix. This patch fixes the problem we saw earlier for selinux issue, sadly not fixing atomic run firewalld firewall-cmd problem.
I will report further related failure details in the firewalld patch. But I think the atomic run related failures are not related to this patch, so I think patch works =).

giuseppe · 2017-12-18T12:21:49Z

@rhatdan are you fine with this PR?

rhatdan · 2017-12-18T14:16:55Z

Well this will cause the target to be labeled correctly and all of the content to have that label. Any content created under the label may or may not be labeled correctly.

So if you are copying a single file this will be fine, if you are copying a directory it could be a problem.

giuseppe · 2017-12-18T16:30:02Z

we copy each file separately so that selabel_lookup_raw+setfscreatecon_raw is done for each file. Doesn't that solve the problem with the directories labelling?

rhatdan · 2017-12-18T16:31:33Z

Yes that will solve the problem.

rhatdan · 2017-12-18T16:31:38Z

LGTM

rhatdan · 2017-12-18T16:31:45Z

@rh-atomic-bot r+

rh-atomic-bot · 2017-12-18T16:31:45Z

📌 Commit 4f9e964 has been approved by rhatdan

rh-atomic-bot · 2017-12-18T16:31:50Z

⌛ Testing commit 4f9e964 with merge 3241b63...

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1143 Approved by: rhatdan

rh-atomic-bot · 2017-12-18T16:49:53Z

☀️ Test successful - status-papr
Approved by: rhatdan
Pushing 3241b63 to master...

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: projectatomic#1143 Approved by: rhatdan

giuseppe mentioned this pull request Dec 6, 2017

WIP: firewalld system containers projectatomic/atomic-system-containers#150

Open

cgwalters reviewed Dec 6, 2017

View reviewed changes

peterbaouoft reviewed Dec 7, 2017

View reviewed changes

giuseppe force-pushed the syscontainers-set-label branch from fadcd8d to 412e981 Compare December 8, 2017 11:59

giuseppe changed the title ~~syscontainers: set selinux label for files copied to the host~~ [WIP] syscontainers: set selinux label for files copied to the host Dec 8, 2017

peterbaouoft reviewed Dec 8, 2017

View reviewed changes

giuseppe force-pushed the syscontainers-set-label branch 3 times, most recently from 774cb72 to 64b8eee Compare December 11, 2017 10:45

giuseppe changed the title ~~[WIP] syscontainers: set selinux label for files copied to the host~~ syscontainers: set SELinux label for files copied to the host Dec 11, 2017

giuseppe force-pushed the syscontainers-set-label branch 7 times, most recently from e65ff3e to 7ea2756 Compare December 11, 2017 13:18

giuseppe added 2 commits December 14, 2017 14:28

syscontainers: set selinux label for files copied to the host

da6e536

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

tests: check copied files have the correct SELinux label

4f9e964

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the syscontainers-set-label branch from 7ea2756 to 4f9e964 Compare December 14, 2017 13:28

rh-atomic-bot pushed a commit that referenced this pull request Dec 18, 2017

tests: check copied files have the correct SELinux label

3241b63

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1143 Approved by: rhatdan

rh-atomic-bot closed this in e8814e6 Dec 18, 2017

eyusupov pushed a commit to eyusupov/atomic that referenced this pull request Mar 10, 2018

syscontainers: set selinux label for files copied to the host

d6dd22f

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: projectatomic#1143 Approved by: rhatdan

eyusupov pushed a commit to eyusupov/atomic that referenced this pull request Mar 10, 2018

tests: check copied files have the correct SELinux label

8e34373

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: projectatomic#1143 Approved by: rhatdan

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

syscontainers: set SELinux label for files copied to the host #1143

syscontainers: set SELinux label for files copied to the host #1143

giuseppe commented Dec 6, 2017

giuseppe commented Dec 6, 2017

cgwalters Dec 6, 2017

giuseppe Dec 8, 2017

peterbaouoft Dec 7, 2017 •

edited

Loading

giuseppe Dec 8, 2017

giuseppe commented Dec 8, 2017

peterbaouoft Dec 8, 2017

giuseppe Dec 8, 2017

giuseppe commented Dec 15, 2017

peterbaouoft commented Dec 15, 2017 •

edited

Loading

giuseppe commented Dec 18, 2017

rhatdan commented Dec 18, 2017

giuseppe commented Dec 18, 2017

rhatdan commented Dec 18, 2017

rhatdan commented Dec 18, 2017

rhatdan commented Dec 18, 2017

rh-atomic-bot commented Dec 18, 2017

rh-atomic-bot commented Dec 18, 2017

rh-atomic-bot commented Dec 18, 2017

syscontainers: set SELinux label for files copied to the host #1143

syscontainers: set SELinux label for files copied to the host #1143

Conversation

giuseppe commented Dec 6, 2017

giuseppe commented Dec 6, 2017

cgwalters Dec 6, 2017

Choose a reason for hiding this comment

giuseppe Dec 8, 2017

Choose a reason for hiding this comment

peterbaouoft Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

giuseppe Dec 8, 2017

Choose a reason for hiding this comment

giuseppe commented Dec 8, 2017

peterbaouoft Dec 8, 2017

Choose a reason for hiding this comment

giuseppe Dec 8, 2017

Choose a reason for hiding this comment

giuseppe commented Dec 15, 2017

peterbaouoft commented Dec 15, 2017 • edited Loading

giuseppe commented Dec 18, 2017

rhatdan commented Dec 18, 2017

giuseppe commented Dec 18, 2017

rhatdan commented Dec 18, 2017

rhatdan commented Dec 18, 2017

rhatdan commented Dec 18, 2017

rh-atomic-bot commented Dec 18, 2017

rh-atomic-bot commented Dec 18, 2017

rh-atomic-bot commented Dec 18, 2017

peterbaouoft Dec 7, 2017 •

edited

Loading

peterbaouoft commented Dec 15, 2017 •

edited

Loading